test(harness): complete the auth mock in the definition list wiring test [SAP-3214] - #899

No actionable comments were generated in the recent review. 🎉

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{A rabbit guards the auth-file door
Real exports hop safely through once more
Missing credentials wait in line
Mocked paths stay fixed and fine
Tests twitch their noses: all is sure}

_{Comment @coderabbitai help to get the list of available commands.}

Review — PR #899 (round 1)

Test-only change to packages/harness/src/server/definition-list-enrichment.test.ts. Nothing published changes: harness ships files: ["dist", …] and tests are not in dist, so no changeset is needed and no prose in this diff reaches an npm consumer. No confidentiality issues — the diff names no company, no private host, and no business arrangement.

1. The same landmine is left armed in the sibling test — packages/harness/src/server/auth-mcp-wiring.test.ts:32

Only two tests boot startServer() behind a wholesale vi.mock("@sapiom/mcp/auth", () => ({ … })): the one fixed here, and auth-mcp-wiring.test.ts, whose factory still enumerates exports with no importOriginal spread. That is exactly the shape that reddened main when src/server/index.ts began importing credentialsFilePath — the next auth import added to the server (which already pulls resolveEnvironment, readCredentialsOrThrow, performBrowserAuth, credentialsFilePath from that module) fails there with the identical No "<export>" export is defined on the "@sapiom/mcp/auth" mock, and main goes red again the same way. The PR fixes one of the two instances of a class bug. Apply the same ...(await importOriginal<typeof import("@sapiom/mcp/auth")>()) spread to auth-mcp-wiring.test.ts — its authFixture already carries credentialsPath and every override it needs, so the spread is a one-line addition.

(The other three @sapiom/mcp/auth mocks — auth-routes.test.ts, profiles/system-prompt-fetch.test.ts, cli/auth.test.ts — import narrow modules rather than the server graph, so they are not exposed to this and do not need touching.)

2. The spread trades a loud failure for a silent real-filesystem read

This is the deliberate point of the change, but the cost is worth naming in the file: with the spread, a future server import of a new real export from @sapiom/mcp/auth no longer fails — it silently runs the real implementation, and every function in that module resolves through credentialsFilePath() = path.join(os.homedir(), ".sapiom", "credentials.json") (packages/mcp/src/credentials.ts:63). So the failure mode becomes "the test quietly reads or writes the developer's real credential store", while the file's own header (lines 1–6) still promises "@sapiom/mcp/auth mocked so the disconnect route never touches ~/.sapiom".

The repo already has the idiom that makes the real code path safe instead of merely absent — redirect the home directory: vi.mock("node:os", async (importOriginal) => ({ ...(await importOriginal()), homedir: () => tmpDir })) at packages/harness/src/core/inject/mcp-config.test.ts:8. With that, the real credentialsFilePath() already points into a temp dir, the explicit credentialsFilePath override and the authFixture.credentialsPath literal both become unnecessary, and any future real export stays contained. At minimum, update the header comment so it states the mock is now partial.

I verified the change's own claim holds today: the mocked path's parent ($TMPDIR/sap3214-enrichment-missing) does not exist, observeCredentialStore watches dirname(filePath) (packages/harness/src/core/credential-store-observer.ts:45), so the watch throws ENOENT, onError filters ENOENT, onUnavailable nulls the observer, and nothing is watched or written. The inline comment is accurate. I could not execute the suite here (build/test commands were not approved in this environment), so I did not independently reproduce the pass.

Nit

• process.env.TMPDIR ?? "/tmp" does not catch TMPDIR="" (path becomes root-relative); harmless here since the path must not exist either way, but the sibling test's plain /tmp/... literal is simpler and already the convention.

Verdict: correct, minimal fix for the red main; land it, then apply the same spread to auth-mcp-wiring.test.ts so the next auth import does not reintroduce the failure.

Review — PR #899 (round 2, delta since 567945d1)

The push added the same importOriginal spread to auth-mcp-wiring.test.ts, rewrote the header comment, and dropped the TMPDIR expression. Still test-only, still nothing published, no confidentiality issues.

New finding — the containment claim in the new comment is false

packages/harness/src/server/definition-list-enrichment.test.ts:26-27: "A new real export would resolve paths through the mocked credentialsFilePath, so it stays contained."

It would not. vi.mock replaces the module's export binding; it does not rewrite calls made inside packages/mcp/src/credentials.ts, where readCredentialsFileOrThrow (line 82) and writeCredentialsFile (line 100) call the module-local credentialsFilePath() — the real one, path.join(os.homedir(), ".sapiom", "credentials.json") (line 63). Today every export of @sapiom/mcp/auth is overridden, so nothing real is reachable and the test is safe. The moment a new export is added to that module and left unmocked, it reads or writes the developer's (or the CI runner's) real credential store — and this comment tells the next reader it cannot. Either delete the claim and say plainly that safety depends on every export being overridden, or make it true by redirecting the home dir: vi.mock("node:os", async (importOriginal) => ({ ...(await importOriginal()), homedir: () => tmpDir })), the idiom already used at packages/harness/src/core/inject/mcp-config.test.ts:8.

Earlier findings

• Round-1 cleanup HTTP SDKs #1 (sibling auth-mcp-wiring.test.ts left unspread) — fixed, spread applied with overrides after ...actual.
• Round-1 update SDKs to use payment protocol data #2 (silent real-filesystem read once the mock is partial) — not fixed; the header now says PARTIAL, which was the minimum asked, but the mitigation was replaced by the incorrect rationale above.
• Round-1 nit (TMPDIR="") — fixed.
• Nothing in round 1 needs correcting.

Verdict: fix or delete the two-line rationale comment; the code itself is correct and ready to land.